node:crypto: default to SHA256 for EC keys in one-shot sign/verify with null algorithm by robobun · Pull Request #33753 · oven-sh/bun

robobun · 2026-07-08T11:35:11Z

What does this PR do?

Fixes crypto.sign(null, data, ecKey) and crypto.verify(null, data, ecKey, sig) throwing ERR_OSSL_NO_DEFAULT_DIGEST instead of defaulting to SHA256 like Node.js.

Reproduction

import crypto from "node:crypto";
const { publicKey, privateKey } = crypto.generateKeyPairSync("ec", { namedCurve: "prime256v1" });
const data = Buffer.from("payload");

const sig = crypto.sign(null, data, privateKey);
console.log(crypto.verify(null, data, publicKey, sig));

Node v26.3.0 prints true. Bun throws:

error:06000077:public key routines:OPENSSL_internal:NO_DEFAULT_DIGEST

The same happens for undefined, for all EC curves (P-256/384/521), for the async callback variants, and for crypto.verify(null, ..., <bad sig>) which should return false but throws.

Cause

When algorithm is null/undefined, Node passes a NULL digest to EVP_DigestSignInit. OpenSSL v3 then queries the key's OSSL_PKEY_PARAM_DEFAULT_DIGEST and receives "SHA256" from the EC keymgmt provider (providers/implementations/keymgmt/ec_kmgmt.c), same as it does for RSA and DSA.

BoringSSL has no default-digest mechanism: do_sigver_init in crypto/fipsmodule/digestsign/digestsign.cc.inc fails with EVP_R_NO_DEFAULT_DIGEST whenever type == NULL and the key uses a prehash method (EC, RSA, DSA).

Bun already compensates for RSA by explicitly selecting SHA256 when the algorithm argument is nullish. EC and DSA were missing from that branch.

Fix

Extend the existing SHA256 default in SignJobCtx::fromJS to cover isSigVariant() (EC and DSA) in addition to isRsaVariant(). This matches OpenSSL v3 behaviour exactly: verified that in Node, crypto.sign(null, data, ecKey) cross-verifies with crypto.verify("sha256", data, ecKey, sig) and vice versa.

Verification

Added EC coverage (P-256/384/521, sync + async, DER + ieee-p1363, bad-signature-returns-false) to test/regression/issue/11029-crypto-verify-null-algorithm.test.ts alongside the existing RSA/Ed25519 cases.

USE_SYSTEM_BUN=1 bun test: 18 new EC tests fail with ERR_OSSL_NO_DEFAULT_DIGEST, 5 existing tests pass
bun bd test: 23/23 pass
test/js/node/test/parallel/test-crypto-sign-verify.js and related crypto sign tests still pass

…y with null algorithm When crypto.sign(null, data, ecKey) or crypto.verify(null, ...) is called with an EC or DSA key, OpenSSL v3 (used by Node.js) queries the key's default digest via OSSL_PKEY_PARAM_DEFAULT_DIGEST and receives SHA256. BoringSSL has no such fallback and fails EVP_DigestSignInit with EVP_R_NO_DEFAULT_DIGEST. Bun already worked around this for RSA keys by explicitly selecting SHA256 when the algorithm argument is null or undefined. Extend the same treatment to EC and DSA keys (isSigVariant) so the one-shot sign and verify paths match Node.js.

robobun · 2026-07-08T11:35:21Z

^{Updated 8:39 AM PT - Jul 8th, 2026}

❌ @robobun, your commit 8d442cc has 4 failures in Build #70479 (All Failures):

test/js/bun/webview/webview.test.ts - code 1 on 🍎 14 aarch64
test/js/web/websocket/autobahn.test.ts - code 1 on 🍎 14 aarch64
test/js/sql/postgres-invalid-message-length.test.ts - code 1 on 🪟 2019 x64
test/js/sql/postgres-binary-array-bounds.test.ts - code 1 on 🪟 2019 x64-baseline

🧪 To try this PR locally:

bunx bun-pr 33753

That installs a local version of the PR into your bun-33753 executable, so you can run:

bun-33753 --bun

coderabbitai · 2026-07-08T11:39:17Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 59df160d-1206-4658-b5f5-cc36abe61356

📥 Commits

Reviewing files that changed from the base of the PR and between 332f744 and 8d442cc.

📒 Files selected for processing (2)

src/jsc/bindings/node/crypto/CryptoSignJob.cpp
test/regression/issue/11029-crypto-verify-null-algorithm.test.ts

Walkthrough

Modifies the default digest selection logic in CryptoSignJob.cpp so SHA256 is applied when no algorithm is specified for both RSA and other signature variants (e.g., EC/DSA), not just RSA. Updates the regression test to replace RSA/Ed25519 assertions with parametrized EC sign/verify test coverage.

Changes

EC/DSA default digest fix

Layer / File(s)	Summary
Default digest condition update `src/jsc/bindings/node/crypto/CryptoSignJob.cpp`	Extends the comment on OpenSSL v3 default-digest behavior to cover EC/DSA and changes the digest-assignment condition from `isRsaVariant()` to `isRsaVariant() \|\| isSigVariant()`.
EC regression test coverage `test/regression/issue/11029-crypto-verify-null-algorithm.test.ts`	Adds `promisify` import and replaces prior RSA/Ed25519 tests with parametrized sync/async EC tests for `null`/`undefined` algorithms, cross-verification defaulting to SHA256, IEEE-P1363 encoding, and invalid-signature verification returning `false`.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main change: defaulting one-shot EC sign/verify to SHA256 when the algorithm is null.
Description check	✅ Passed	The description matches the template and includes the change summary plus a verification section.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{Comment @coderabbitai help to get the list of available commands.}

claude

I didn't find any issues — the change is a one-line extension of the existing RSA SHA256-default to EC/DSA and is well-tested — but since it's in the node:crypto sign/verify path I'm leaving it for a human to sign off.

Extended reasoning...

Overview

This PR extends an existing BoringSSL/OpenSSL compatibility shim in SignJobCtx::fromJS (src/jsc/bindings/node/crypto/CryptoSignJob.cpp). Previously, when crypto.sign()/crypto.verify() were called with a null/undefined algorithm, Bun explicitly defaulted to SHA256 only for RSA keys (to match OpenSSL v3's OSSL_PKEY_PARAM_DEFAULT_DIGEST behavior that BoringSSL lacks). The fix adds || isSigVariant() to that condition, which covers EVP_PKEY_EC and EVP_PKEY_DSA — verified in ncrypto.cpp:2781-2786. The rest of the diff is comment updates and 18 new test cases in the existing regression test file covering P-256/384/521, sync/async, DER/ieee-p1363 encoding, and cross-verification against explicit "sha256".

Security risks

None identified. The change does not weaken any verification path — it makes a previously-throwing call succeed with a secure, well-established default (SHA256), exactly mirroring Node.js/OpenSSL v3 behavior. The cross-verification tests confirm that sign(null, ...) output verifies against verify("sha256", ...) and vice versa, so the digest selection is correct rather than merely self-consistent. Ed25519/Ed448 (one-shot variants) remain unaffected since isSigVariant() is false for them.

Level of scrutiny

The native change is mechanically trivial (one added disjunct on an existing condition) and follows the exact pattern already in place for RSA. However, it lives in the node:crypto sign/verify implementation, which is security-sensitive by policy. A quick human confirmation that SHA256 is the right default for EC/DSA under OpenSSL v3 (the PR cites ec_kmgmt.c and dsa_kmgmt.c) is warranted.

Other factors

The PR description is thorough with upstream source references, the tests include negative cases (bad signature → false, wrong data → false), and the author confirmed the new tests fail on system Bun and pass on the debug build. No prior reviews or outstanding comments on the PR.

robobun · 2026-07-08T11:55:06Z

CI failures on build 70479 are unrelated to this change:

test/js/sql/postgres-binary-array-bounds.test.ts and test/js/sql/postgres-invalid-message-length.test.ts fail with ERR_POSTGRES_CONNECTION_REFUSED on Windows 2019. Same failure on builds 70473, 70468, 70465, 70464, 70456 across unrelated branches, so this is a Windows postgres service issue on the fleet.
Flaky-annotation entries (update_interactive_install.test.ts, zlib/leak.test.ts) passed on retry.

This diff touches only CryptoSignJob.cpp and the 11029 regression test. The new EC tests pass on every lane that ran them. Ready for review.

github-actions Bot added the claude label Jul 8, 2026

claude Bot reviewed Jul 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

node:crypto: default to SHA256 for EC keys in one-shot sign/verify with null algorithm#33753

node:crypto: default to SHA256 for EC keys in one-shot sign/verify with null algorithm#33753
robobun wants to merge 1 commit into
mainfrom
claude/farm/52a90450/ec-sign-null-digest

robobun commented Jul 8, 2026

Uh oh!

robobun commented Jul 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 8, 2026

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 8, 2026

What does this PR do?

Reproduction

Cause

Fix

Verification

Uh oh!

robobun commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 8, 2026

Walkthrough

Changes

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 8, 2026 •

edited

Loading